Fix inbetween-inserter position in RTL languages #49683
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Bug
|
Size Change: +1 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f9f8c4e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4656720564
|
ciampo
reviewed
Apr 11, 2023
| // vertical, ltr | ||
| left = previousRect ? previousRect.left : nextRect.left; | ||
| } | ||
| left = previousRect ? previousRect.left : nextRect.left; |
There was a problem hiding this comment.
Out of curiosity, why was the isRTL() check here unnecessary?
There was a problem hiding this comment.
just imagine this visually.
In LTR languages, the previous element is on the left of the next element but in RTL it's on the right. meaning that if we want to position a "div" between the two elements, its "left" coordinate should be the "right" coordinate of the "previous" element in LTR but it should be the "left" coordinate of the "next" element in RTL.
There was a problem hiding this comment.
Oh I guess the question here is the "opposite". It's unnecessary here because in the case of the "vertical" in-between inserter, the "left" position is the same for both blocks anyway, so the left position of the empty space between them is also the same "left" position, so either prev.left or next.left. (and shouldn't be right in any case)
There was a problem hiding this comment.
Thank you for the explanation — it makes sense to remove the RTL check for the vertical inserter :)
ciampo
approved these changes
Apr 11, 2023
| // vertical, ltr | ||
| left = previousRect ? previousRect.left : nextRect.left; | ||
| } | ||
| left = previousRect ? previousRect.left : nextRect.left; |
There was a problem hiding this comment.
Thank you for the explanation — it makes sense to remove the RTL check for the vertical inserter :)
ndiego
added
the
[Feature] UI Components
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #30867
What?
This PR fixes the position of the inbetween inserter in both vertical and horizontal position for RTL languages.
Testing Instructions
1- Switch to an RTL language (like arabic)
2- Insert two paragraphs and verify that you can trigger the in-between inserter at the right position
3- Insert a social links block with at least two links and verify that you can trigger the in-between horizontal inserter between two buttons.
I also noticed that if you use the "slash" inserter, the initial position is correct but as soon as you start typing, the position is wrong. I think that's unrelated with the issue being fixed here though and might be some race condition within "Popover" component.